-
Notifications
You must be signed in to change notification settings - Fork 1.4k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
libvirt: Add Terraform variables for bootstrap node memory/CPU #852
Conversation
6056f2f
to
a55a161
Compare
Would be nice to have the |
The bootstrap node is done and gone before the heavy monitoring stuff rolls out. Is yours OOMing anyway? |
@wking It's not OOMing, more so I'm just trying to give it more resources to speed things up a bit. |
/test e2e-aws |
I'd expect we're network-limited by all the images we pull (especially if you aren't installing from a local registry). Can you post some benchmarks of time until |
+1 I don't think we should add knobs for bootstrap node. If we see that we need more memory lets just use that. |
@sjug I don't think we can accept this change of adding this tunable. But if you have some metrics that the current setup of bootstrap node is not adequate in terms of cpu / memory we can accept changing the defaults in this PR itself or close this one and open another with change to defaults. |
The install time is not very consistent so it was a bit challenging to measure any improvement or lack of. The times are an average of two runs, with large std dev. As we increase cores:
As we increase memory:
That formatting was painful, apologies. Do we want to change the hardcoded default? |
It looks like we should change the number of cores from two to four on the bootstrap domain. Is that valid for hosts which have less than four cores? Either way, I think we can close this PR in favor of changing the default. |
Memory should have a reasonable cap, but won't CPUs just be "the more, the merrier"? I don't know if bootstrap CPU allocation is something we want to try to fine-tune.
Check by setting it to 256? If that works, maybe we should make 256 the default ;). |
Sanest thing is to match the host. On this topic, see https://pagure.io/standard-test-roles/pull-request/223 |
What about the worker nodes, btw? Any thought on their sizing? |
Worker nodes are created by the libvirt cluster-API provider, not by Terraform. So adjust their MachineSet objects just like you'd manage other Kubernetes resources (and ping the libvirt-provider maintainers if something isn't working ;). |
@wking - how do I fix the 1st worker though? It's created and overloaded with 93% of its memory consumed! I can ping them of course. |
We shouldn't distract here. Spin off to a libvirt-provider or separate installer issue? There may already be issues discussing this; I haven't checked |
@cgwalters I'm not aware of any way to make terraform variables dynamic to get the host CPU count |
a55a161
to
f82c0ca
Compare
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: sjug If they are not already assigned, you can assign the PR to them by writing The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
No longer have any variables for bootstrap node cpu or memory, closing. |
@sjug: The following test failed, say
Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here. |
Similar to #785, but for the bootstrap node.